Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Install test #64

Merged
merged 37 commits into from
Feb 29, 2024
Merged

Install test #64

merged 37 commits into from
Feb 29, 2024

Conversation

yasahi-hpc
Copy link
Collaborator

@yasahi-hpc yasahi-hpc commented Feb 20, 2024

Resolves #43
Resolves #66

This PR aims at recovering CI for using KokkosFFT as subdirectory or library.

  1. Build app using KokkosFFT as a subdirectory in CMake project
  2. Build app using Kokkos and KokkosFFT as libraries in CMake project
  3. Add these tests in CI
  4. fix CMake issues when using KokkosFFT as a subdirectory
  5. docs would be updated based on the changes

@yasahi-hpc yasahi-hpc self-assigned this Feb 21, 2024
@yasahi-hpc yasahi-hpc requested a review from pzehner February 21, 2024 07:25
Copy link
Collaborator

@pzehner pzehner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on our previous discussion, testing the installation of KokkosFFT should be done in a different job:

install:
  # same `needs` and `if` as the `build` job.
  # the `strategy` may be similar, without testing host-device configuration (i.e. drop
  # the `target` matrix), and maybe without some configurations (i.e. no threads and
  # serial?)

The content of the files as_subdirectory.sh and as_library.sh should appear within the test as maybe two separate steps. Having script files lying around in the source tree that are only used for CI is not a good thing. Instead of re-downloading Kokkos, I think we can re-use the one in /tpls by copying it (which makes it consistent with the build tests).

I don't like the idea to duplicate the install test file, but I think it's better to keep two separate directories for the library and the subdirectory approaches, as they require different CMakeLists.txt files. Maybe just reduce the test file to its simplest expression.

@yasahi-hpc
Copy link
Collaborator Author

@pzehner Thanks for the comments

Based on our previous discussion, testing the installation of KokkosFFT should be done in a different job:

install:
  # same `needs` and `if` as the `build` job.
  # the `strategy` may be similar, without testing host-device configuration (i.e. drop
  # the `target` matrix), and maybe without some configurations (i.e. no threads and
  # serial?)

The content of the files as_subdirectory.sh and as_library.sh should appear within the test as maybe two separate steps. Having script files lying around in the source tree that are only used for CI is not a good thing. Instead of re-downloading Kokkos, I think we can re-use the one in /tpls by copying it (which makes it consistent with the build tests).

That makes sense. I will update accordingly.

I don't like the idea to duplicate the install test file, but I think it's better to keep two separate directories for the library and the subdirectory approaches, as they require different CMakeLists.txt files. Maybe just reduce the test file to its simplest expression.

Do you mean we should modify each CMakeLists.txt to refer to a single source file, say,

add_executable(hello-kokkos-fft ../src/hello.cpp)

@pzehner
Copy link
Collaborator

pzehner commented Feb 22, 2024

Do you mean we should modify each CMakeLists.txt to refer to a single source file, say,

add_executable(hello-kokkos-fft ../src/hello.cpp)

We would end up with the following layout:

install_test
    +-- main.cpp
    +-- as_library
    |   +-- CMakeLists.txt
    +-- as_subdirectory
        +-- CMakeLists.txt

Which is not very common. It's up to you I guess.

@yasahi-hpc
Copy link
Collaborator Author

Do you mean we should modify each CMakeLists.txt to refer to a single source file, say,

add_executable(hello-kokkos-fft ../src/hello.cpp)

We would end up with the following layout:

install_test
    +-- main.cpp
    +-- as_library
    |   +-- CMakeLists.txt
    +-- as_subdirectory
        +-- CMakeLists.txt

Which is not very common. It's up to you I guess.

I may prefer self-contained examples like

 install_test
    +-- as_library
    |   +-- CMakeLists.txt     
    |   +-- hello.cpp
    +-- as_subdirectory
        +-- CMakeLists.txt
        +-- hello_again.cpp

Using slightly different examples. Is it fine with you?

@yasahi-hpc yasahi-hpc requested a review from pzehner February 23, 2024 10:20
Yuuichi Asahi and others added 15 commits February 27, 2024 23:47
Ideally, the two jobs should remain separated. The `install` job would
use almost the same `matrix` (minus `target`) as the `build` job and
have the same `needs` and `if` as the `test` job. This would however
make a duplicated and error-prone `matrix` for the two jobs, and sharing
a `matrix` between jobs is currently not possible with GitHub Actions
(neither directly, nor using YAML anchors, other options were too
 cumbersome when writing this). Consequently, for the sake of
maintainability, the two jobs are merged for now.
@yasahi-hpc yasahi-hpc merged commit 202bed0 into main Feb 29, 2024
19 checks passed
@yasahi-hpc yasahi-hpc deleted the install-test branch February 29, 2024 08:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pertinence of Kokkos_Version_Check Pre-build Kokkos on CI
3 participants